Skip to content

feat(criterion): make Bencher implement Send + Sync #116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(criterion): make Bencher implement Send + Sync #116

wants to merge 1 commit into from

Conversation

DaniPopes
Copy link
Contributor

criterion's Bencher implements Send + Sync, which allows running the benchmark itself on another thread. This allows something like |b| /* my rayon thread pool */.install(|| b.iter(|| /* bench */)).

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Aug 2, 2025 •
edited
Loading

CodSpeed Instrumentation Performance Report

Merging #116 will degrade performances by 3.1%

Comparing DaniPopes:criterion-bencher-send (1f8a754) with main (b211084)

Summary

⚡ 12 improvements
❌ 3 regressions
✅ 152 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
⚡ Iterative 162.5 ns 132.5 ns +22.64%
⚡ Iterative[20] 162.5 ns 132.5 ns +22.64%
⚡ Iterative[21] 164.2 ns 134.2 ns +22.36%
❌ fibonacci_custom_measurement 916.7 ns 945.8 ns -3.08%
⚡ sleep_100ms 624.4 ns 566.1 ns +10.3%
⚡ sleep_10ms 595.3 ns 566.1 ns +5.15%
⚡ sleep_1ms 595.3 ns 566.1 ns +5.15%
⚡ sleep_50ms 624.4 ns 566.1 ns +10.3%
❌ from_elem[1024] 1.8 µs 1.9 µs -3.1%
❌ from_elem_decimal[1024] 1.8 µs 1.9 µs -3.1%
⚡ sleep_100ns 566.1 ns 536.9 ns +5.43%
⚡ sleep_100us 566.1 ns 536.9 ns +5.43%
⚡ sleep_1ms 566.1 ns 536.9 ns +5.43%
⚡ sleep_1ns 566.1 ns 536.9 ns +5.43%
⚡ sleep_1us 566.1 ns 536.9 ns +5.43%

@art049 art049 requested a review from not-matthias August 2, 2025 15:28
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Aug 2, 2025 •
edited
Loading

CodSpeed WallTime Performance Report

Merging #116 will degrade performances by 11.01%

Comparing DaniPopes:criterion-bencher-send (1f8a754) with main (b211084)

Summary

⚡ 11 improvements
❌ 5 regressions
✅ 138 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
⚡ build_vec 207 ns 196 ns +5.61%
⚡ large_drop 174.3 µs 168.1 µs +3.67%
⚡ large_setup 13 ns 12 ns +8.33%
⚡ from_elem[16384] 860 ns 823 ns +4.5%
⚡ from_elem[2048] 244 ns 235 ns +3.83%
❌ from_elem[4096] 316 ns 326 ns -3.07%
⚡ from_elem_decimal[2048] 263 ns 249 ns +5.62%
❌ graph_coloring[3] 1.1 µs 1.3 µs -11.01%
⚡ graph_coloring[5] 1,046 ns 995 ns +5.13%
❌ n_queens_solver[5] 7.3 µs 7.7 µs -4.54%
❌ permutations[4] 2.4 µs 2.5 µs -4.33%
⚡ permutations[5] 14.3 µs 11.1 µs +28.73%
❌ permutations[6] 71.7 µs 75.9 µs -5.53%
⚡ rat_in_maze[5] 599 ns 581 ns +3.1%
⚡ sleep_100ns 39.2 µs 26.3 µs +48.93%
⚡ sleep_1ns 41.7 µs 29.6 µs +41.09%

@DaniPopes DaniPopes mentioned this pull request Aug 4, 2025
Merged
not-matthias
not-matthias approved these changes Aug 7, 2025
View reviewed changes
Copy link
Member

@not-matthias not-matthias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Minor style nitpick: You could remove the let codspeed = &mut *self.codspeed; variables altogether and use it directly when calling the functions (e.g. self.codspeed.start_benchmark)

@DaniPopes
feat(criterion): make Bencher implement Send + Sync
1f8a754 
`criterion`'s `Bencher` implements `Send + Sync`, which allows running
the benchmark itself on another thread. This allows something like
`|b| /* my rayon thread pool */.install(|| b.iter(|| /* bench */))`.
@DaniPopes DaniPopes force-pushed the criterion-bencher-send branch from ad60ffa to 1f8a754 Compare August 7, 2025 21:24
@DaniPopes DaniPopes requested a review from not-matthias August 7, 2025 21:29
@DaniPopes
Copy link
Contributor Author

Any chance this can get merged and released soon? Thanks! :D @art049 @not-matthias

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@not-matthias not-matthias not-matthias approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DaniPopes @not-matthias